Binary parameter handling for GeoDjango #2169
Conversation
for more information, see https://pre-commit.ci
…f docs/changes.rst.
updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.4 → v0.12.5](astral-sh/ruff-pre-commit@v0.12.4...v0.12.5) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
|
@tim-schilling I added some commits following what you were pointing out, let me know what do you think! |
| import json | ||
|
|
||
|
|
||
| class DebugToolbarJSONDecoder(json.JSONDecoder): |
There was a problem hiding this comment.
Rather than the approach you've taken here, you may want to consider going with something like:
import base64
import json
def object_hook(obj):
for key, value in obj.items():
if isinstance(value, str) and value.startswith("__djdt_binary__"):
_, encoded = value.split("__djdt_binary__", maxsplit=1)
obj[key] = base64.b64decode(encoded)
return obj
class DebugToolbarJSONDecoder(json.JSONDecoder):
"""Custom JSON decoder that reconstructs binary data during parsing."""
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
if self.object_hook is None:
self.object_hook = object_hookThis will require you to change the tests. The decoding will only work if there's an actual object being passed in. Most of the tests are passing in a list of values. Using an object/dictionary is more representative of what will actually be used since this is for parameters for a SQL query which is a dictionary.
| # Handle binary data (e.g., GeoDjango EWKB geometry data) | ||
| if isinstance(param, (bytes, bytearray)): | ||
| # Mark as binary data for later reconstruction | ||
| return {"__djdt_binary__": base64.b64encode(param).decode("ascii")} |
There was a problem hiding this comment.
We should move this logic into a shared utility that way we don't have the magic value "__djdt_binary__" floating around the code-base. Plus it would be clearer how the logic gets paired together.
I'm also curious if we should be using this with the Store logic. If we do, then we could potentially ignore this and handle it within DebugToolbarJSONEncoder and then have DebugToolbarJSONDecoder as above. What do you think?
| self.assertEqual(len(reconstructed), 1) | ||
| self.assertEqual(reconstructed[0], binary_data) | ||
| self.assertIsInstance(reconstructed[0], bytes) |
There was a problem hiding this comment.
I don't think there's a benefit to being this verbose. Try to be more concise such as:
| self.assertEqual(len(reconstructed), 1) | |
| self.assertEqual(reconstructed[0], binary_data) | |
| self.assertIsInstance(reconstructed[0], bytes) | |
| self.assertEqual(reconstructed, [binary_data]) |
Co-authored-by: Tim Schilling <[email protected]>
Description
hey folks, this is my very first PR in the django-commons. I did this because I faced the issue myself in one project that I'm working on. I don't know how I can test this in that project. So I will wait for some of you to tell me how I can do that... but in the meantime this is the fix for the SQL Explain functionality for GeoDjango queries with binary parameters.
Fixes #423
SQL Explain feature throws a 500 error when used with GeoDjango queries containing binary parameters. This affects any query using PostGIS functions like ST_GeomFromEWKB(), ST_Distance_Sphere(), etc.
I'm trying to create a robust binary parameter handling system:
Checklist:
docs/changes.rst.